Skip to content

Change from using rand() to thread-safe ap_random_pick. #1289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Change from using rand() to thread-safe ap_random_pick. #1289

wants to merge 1 commit into from

Conversation

bostrt
Copy link

@bostrt bostrt commented Dec 20, 2016 •
edited
Loading

Use of rand() led to possibility of multiple threads performing GC all at once. I temporarily added some debug logging to show this (this pull request does not include logging):

@@ -212,6 +212,7 @@ static void modsecurity_persist_data(modsec_rec *msr) {
     apr_table_entry_t *te;
     apr_time_t time_before, time_after;
     int i;
+    int rvalue;
 
     time_before = apr_time_now();
 
@@ -237,7 +238,9 @@ static void modsecurity_persist_data(modsec_rec *msr) {
     }
 
     /* Remove stale collections. */
-    if (rand() < RAND_MAX/100) {
+    rvalue = rand();
+    if (rvalue < RAND_MAX/100) {
+        msr_log(msr, 9, "Garbage collection initiated with rvalue: %d < %d/100", rvalue, RAND_MAX);
         arr = apr_table_elts(msr->collections);
         te = (apr_table_entry_t *)arr->elts;
         for (i = 0; i < arr->nelts; i++) {

[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][9] Garbage collection initiated with rvalue: 15337631 < 2147483647/100
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][4] Garbage collection took 42 microseconds.
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389ea670][/index.html][9] Garbage collection initiated with rvalue: 15337631 < 2147483647/100
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389ea670][/index.html][4] Garbage collection took 122 microseconds.
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][9] Garbage collection initiated with rvalue: 15337631 < 2147483647/100
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][4] Garbage collection took 74 microseconds.
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][9] Garbage collection initiated with rvalue: 15337631 < 2147483647/100
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][4] Garbage collection took 123 microseconds.
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][9] Garbage collection initiated with rvalue: 15337631 < 2147483647/100
[18/Dec/2016:13:29:29 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][4] Garbage collection took 160 microseconds.
[18/Dec/2016:13:29:36 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][9] Garbage collection initiated with rvalue: 15337631 < 2147483647/100
[18/Dec/2016:13:29:36 --0500] [localhost/sid#5621386b7400][rid#5621389e8660][/index.html][4] Garbage collection took 53 microseconds.
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d04d0][/index.html][9] Garbage collection initiated with rvalue: 6841472 < 2147483647/100
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d04d0][/index.html][4] Garbage collection took 63 microseconds.
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d24e0][/index.html][9] Garbage collection initiated with rvalue: 6841472 < 2147483647/100
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d24e0][/index.html][4] Garbage collection took 68 microseconds.
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d04d0][/index.html][9] Garbage collection initiated with rvalue: 6841472 < 2147483647/100
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d04d0][/index.html][4] Garbage collection took 43 microseconds.
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d04d0][/index.html][9] Garbage collection initiated with rvalue: 6841472 < 2147483647/100
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d04d0][/index.html][4] Garbage collection took 114 microseconds.
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d04d0][/index.html][9] Garbage collection initiated with rvalue: 6841472 < 2147483647/100
[18/Dec/2016:13:29:37 --0500] [localhost/sid#5621386b7400][rid#5621389d04d0][/index.html][4] Garbage collection took 46 microseconds.

Notice multiple threads having same rand() result.

Finally, changing to ap_random_pick, as mentioned in #1286, resulted in no duplicate runs of GC:

[18/Dec/2016:13:33:08 --0500] [localhost/sid#56385f3c6400][rid#56385f6ed5d0][/index.html][9] Garbage collection initiated with rvalue: 8426056 < 2147483647/100
[18/Dec/2016:13:33:08 --0500] [localhost/sid#56385f3c6400][rid#56385f6ed5d0][/index.html][4] Garbage collection took 58 microseconds.
[18/Dec/2016:13:33:10 --0500] [localhost/sid#56385f3c6400][rid#56385f6e14a0][/index.html][9] Garbage collection initiated with rvalue: 12418543 < 2147483647/100
[18/Dec/2016:13:33:10 --0500] [localhost/sid#56385f3c6400][rid#56385f6e14a0][/index.html][4] Garbage collection took 87 microseconds.
[18/Dec/2016:13:33:11 --0500] [localhost/sid#56385f3c6400][rid#56385f6ef5e0][/index.html][9] Garbage collection initiated with rvalue: 7706903 < 2147483647/100
[18/Dec/2016:13:33:11 --0500] [localhost/sid#56385f3c6400][rid#56385f6ef5e0][/index.html][4] Garbage collection took 87 microseconds.
[18/Dec/2016:13:33:12 --0500] [localhost/sid#56385f3c6400][rid#56385f6f7620][/index.html][9] Garbage collection initiated with rvalue: 15493977 < 2147483647/100
[18/Dec/2016:13:33:12 --0500] [localhost/sid#56385f3c6400][rid#56385f6f7620][/index.html][4] Garbage collection took 53 microseconds.

@zimmerle zimmerle self-assigned this May 8, 2017
@zimmerle zimmerle self-requested a review May 8, 2017 22:01
zimmerle pushed a commit that referenced this pull request May 9, 2017
Adds info about pull request #1289
4b5a635
zimmerle pushed a commit that referenced this pull request May 9, 2017
Fix compilation for 2.2.x after #1289
42af67d
zimmerle pushed a commit that referenced this pull request May 11, 2017
Fix compilation for 2.2.x and standalone after #1289
a5bbb83
@zimmerle
Copy link
Contributor

Hi @bostrt,

Thank you for the patch! Sorry for the long delay. I've made some adjustments to have the code compiling for our standalone version. Standalone will be using rand().

Patch is merged ;)

@zimmerle zimmerle closed this May 11, 2017
@zimmerle zimmerle mentioned this pull request May 21, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimmerle zimmerle Awaiting requested review from zimmerle

Assignees

@zimmerle zimmerle

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bostrt @zimmerle